Bound the BLE connect and file-read waits so they report instead of hanging - #546
Draft
dhalbert wants to merge 5 commits into
Draft
Bound the BLE connect and file-read waits so they report instead of hanging#546dhalbert wants to merge 5 commits into
dhalbert wants to merge 5 commits into
Conversation
`connectToBluetoothDevice()` had two unbounded waits, and on Linux both of them hang. The connect dialog stays open with no feedback and no error. First, it waited for an `advertisementreceived` event before connecting at all. Chrome's BlueZ backend never delivers that event: measured 0 events in 45s while BlueZ concurrently received 38 advertising reports from the same device. The same page on macOS gets its first event ~30ms after arming. So on Linux the connect was never even attempted. The wait is now bounded by `ADVERTISEMENT_WAIT_MS`, and we connect anyway when it expires. Second, `gatt.connect()` itself does not always reject. Chrome bounds it at ~41s on Linux normally, but not while a `watchAdvertisements()` watch is armed -- in that state the promise simply never settles, observed over two minutes with no connection attempt in progress at the BlueZ level. It is now raced against `CONNECT_TIMEOUT_MS` and cancelled with `gatt.disconnect()`, which is the only way page JS can abort an in-flight connect. Failure produces an actionable message and re-enables the button. The watch is deliberately left armed until the connect settles, rather than aborted first as before. On Linux the kernel only takes the working connect path while a discovery session is active -- `hci_update_passive_scan_sync()` returns early when `discovery.state != DISCOVERY_STOPPED`, and otherwise installs an accept-list-filtered passive scan that never matches -- and Chrome holds a discovery session for the lifetime of the watch. Other devices' watches are still aborted immediately so Chrome's per-device watch quota is not consumed. Adds `_connectAttemptInFlight` so that several remembered devices whose advertisement waits expire together cannot all try to connect at once. None of this makes Linux reliable; that needs a host fix. It converts an indefinite silent hang into a bounded, reported failure. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reading device info over BLE could hang forever, leaving the editor spinning
on "Current Device Info" with no way out but a reload. Reproduced on Linux by
letting pairing fail: the connect succeeds, encryption then drops the link,
and the device-info read is issued on a dead connection.
The defect is upstream in `@adafruit/ble-file-transfer-js`. `readFile()` and
`listDir()` install their promise's reject handler *after* writing the
request:
await this._write(header);
await this._write(encoded);
let p = new Promise((resolve, reject) => {
this._resolve = resolve;
this._reject = reject; // too late
});
return p;
On a dead link `_transfer` is null, so both writes throw. `_write()` swallows
the error and calls `onDisconnected()`, which has no `_reject` to call yet.
`checkConnection()` likewise catches its own failure and returns normally
rather than rethrowing, so the read proceeds regardless. The returned promise
is then never settled by anyone.
Rather than patch upstream from here, our `FileTransferClient` wrapper guards
the two read paths with `_whileConnected()`: reject immediately if the GATT
link is already down, and reject if it drops while the read is in flight.
Bounding on liveness rather than elapsed time is deliberate -- a large file
read over BLE can legitimately take tens of seconds, so a stopwatch would
produce false failures, while a dropped link is unambiguous. The mutating ops
are left alone, since they are meant to span the autoreload disconnect (circuitpython#377).
That alone stops the hang, because `showBusy()` clears the spinner in a
`finally`. But the rejection then escaped `_getVersionInfo()` and
`_getDeviceInfo()` uncaught, leaving a blank dialog that reads as "the device
answered with nothing". Both now catch and show a message, using the
`#message` element the other modals already use, added to these two.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two follow-ups to bounding the connect, both about how the wait feels rather than what it does. `ADVERTISEMENT_WAIT_MS` drops from 5s to 2s. On Linux the event never arrives, so the wait always runs to the full timeout before the connect is attempted, and five seconds of it is pure latency. It is not wasted time though: the discovery session that `watchAdvertisements()` opens is what makes BlueZ create its device object, without which `gatt.connect()` rejects immediately as "no longer in range". A second or so is enough for that, and platforms where the event does arrive get it in about 30ms, so the constant is irrelevant to them. The wait was also completely silent, because `clearConnectStatus()` runs just before it. Two to five seconds of a blank dialog reads as a hang, which is the impression this whole change set is trying to remove, so show "Looking for <device>..." until the connect starts. Untested against hardware: the Linux connect only succeeds about a third of the time for unrelated host reasons, which makes the latency difference hard to observe deliberately. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The previous commit bounded gatt.connect() in connectToBluetoothDevice() but missed the copy in _attemptSilentReconnect(), which is arguably the worse of the two. CircuitPython autoreloads after every mutating file operation, which drops the link, so that reconnect ladder runs after every save. An unbounded connect there stalls the ladder, and the mutating op waits on it through awaitPostOpReconnect(), so a save spins with no way out. Extracts the timeout-and-cancel race into _connectWithTimeout(device, ms) and uses it in both places, rather than repeating it. The silent path gets its own shorter bound. CONNECT_TIMEOUT_MS is 30s, chosen so a slow-but-real Linux connect is not abandoned; three of those in the reconnect ladder would be 90s of apparent hang. Ten seconds is long enough for a reconnect that is going to work -- post-autoreload reconnects land in about a second -- and past that it has stopped being silent anyway, so failing over to the manual reconnect UI is the better outcome. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
Claude wrote this: Noting a related gap found during the same testing, not addressed in this PR.
async readOnly() {
let readonly = false;
return false;
// Check if the device is read only
console.log("Checking if device is read only");
// Attempt to write a 0-byte temp file and remove it
const testPath = '/._ble_readonly_check';
...
}Everything after the The consequence is that when the board's filesystem is read-only to CircuitPython, which is the default whenever USB MSC is active, the editor reports it as writable and goes ahead with saves that cannot succeed. The device answers This is worth mentioning here because it made the failure hard to attribute during testing: repeated save failures over BLE looked like a bug in the write path, and it took a hex dump of the result to establish that nothing had been written at all and why. The real check is not free — it writes and deletes a temp file on every connect — so short-circuiting it may well be deliberate. If so, the reporting is still worth improving: |
Chrome holds a BlueZ discovery session for as long as any watchAdvertisements() watch is armed, and connecting while one is active is what fails on Linux -- the opposite of what the previous comment claimed. Driving Device1.Connect() directly: 36/36 with discovery stopped, 18/52 with it active. _abortAdvWatches() now drops this device's own watch too, and the redundant call in the finally block goes away since nothing is left pending by then. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 Generated with Claude Code
Draft: the ordering change below is not yet verified in a browser. See Testing. Struck-through text was wrong and is superseded by the measurements that follow it.
The BLE connect flow had several waits with no upper bound, so a failure that should have been reported instead left the editor sitting there indefinitely. This does not make any of the underlying failures go away — most of them are host-side and not ours to fix — it makes them report instead of hang.
What was unbounded
Waiting for an advertisement before connecting.
connectToBluetoothDevice()armedwatchAdvertisements()and only calledgatt.connect()from theadvertisementreceivedhandler. That event is never delivered on Linux: measured 0 events in 45 s while BlueZ concurrently received 38 advertising reports from the same board. It is unimplemented rather than broken — Chromium's BlueZ backend gates advertisement delivery on anEIRD-Bus property that exists only in the ChromeOS fork, Web Bluetooth on Linux is officially "partially implemented and not supported", andwatchAdvertisements()is marked "No longer pursuing". So on Linux the connect was never attempted at all, which is the "device chooser works, nothing happens afterwards" symptom.gatt.connect()itself. Chrome bounds this at roughly 41 s on Linux normally, but not while awatchAdvertisements()watch is armed — in that state the promise simply never settles, observed over two minutes with no connection attempt in progress at the BlueZ level. It is now raced against a timer and cancelled withgatt.disconnect(), which Chrome has honoured as a cancel since M140.The same call in
_attemptSilentReconnect(). Easy to miss and arguably the worse of the two, since CircuitPython autoreloads after every mutating file operation and that reconnect ladder therefore runs after every save. An unbounded connect there stalls the ladder, and the mutating op waits on it throughawaitPostOpReconnect(), so a save spins with no way out.A file read on a dead link.
readFile()andlistDir()could return a promise that nobody can settle, because upstream installs the reject handler after writing the request — see adafruit/ble-file-transfer-js#13. Guarded here on link liveness rather than a stopwatch, since a large file read over BLE can legitimately take tens of seconds while a dropped link is unambiguous. The device-info dialogs also now catch the rejection and say something, instead of leaving a blank dialog that reads as "the device answered with nothing".Deliberate choice worth reviewingAbort the advertisement watch before connectingThe advertisement watch is now left armed across the connect and aborted only afterwards, rather than aborted first. On Linux the kernel only takes its working connect path while a discovery session is active —hci_update_passive_scan_sync()returns early whendiscovery.state != DISCOVERY_STOPPED, and otherwise installs an accept-list-filtered passive scan that never matches — and Chrome holds a discovery session for the lifetime of the watch. Other devices' watches are still aborted immediately so Chrome's per-device watch quota is not consumed.This leans on Chrome-on-Linux implementation detail, not specified behaviour. It is commented as such.That was backwards, and measurement says so. Chrome holds a BlueZ discovery session for as long as any
watchAdvertisements()watch is armed, and connecting while one is active is the failing condition — not the working one.Driving
Device1.Connect()directly over D-Bus, with no browser involved, arm A being discovery active at connect time and arm B discovery stopped:Fisher's exact on the failing state, p ≈ 4 × 10⁻⁷. Arm B has never failed: 36/36 across two controllers, both connect paths, every signal level tried, and both orderings.
So the watch — including this device's own — is now aborted before
gatt.connect(), not held across it. The bounded wait added by the earlier commits is what drives the connect on Linux, since the advertisement event never arrives there anyway.Testing
Feather nRF52840 Express and Circuit Playground Bluefruit, CircuitPython 10.3.0-alpha.4, Chrome 151.
Verified: the bounded connect on Linux, where a failed attempt now reports an actionable error and re-enables the button instead of hanging; and the file-read guard, which stops the "Current Device Info" spinner. Both reproduced before and after.
Not verified against hardware: the shorter advertisement wait with its status message, the bounded silent reconnect, and the abort-before-connect ordering. The ordering is supported by the D-Bus measurements above rather than by browser testing, and there is a known gap:
abort()returns synchronously in page JS but the resultingStopDiscoveryreaches BlueZ asynchronously in the browser process, sogatt.connect()may still fire while the kernel is scanning. The measurements let discovery settle for a second first, so the tight ordering is untested. Hence still draft.Context
The Linux connect unreliability underneath all of this is a host defect, not something the editor can fix.
The kernel frequently issues no create-connection at all for an unbonded peripheral, succeeding on about a third of attempts.What actually happens, frombtmonplus an air capture: the kernel issues a correctLE Extended Create Connection, the controller answersCommand Status: Success, and then transmits nothing on air for 20 s until the attempt is cancelled — noCONNECT_INDreaches the peripheral, which advertises steadily throughout and answers other devices' scan requests in the same window.Two further things worth knowing for anyone trying to reproduce it. The failing state is established by booting and cleared by a host suspend/resume — a 19-second suspend is enough — so on a machine that has been suspended since boot it will not reproduce at all. And it did not reproduce on a CSR dongle in the same host, same boot, bracketed either side by failing runs on the built-in MediaTek controller; whether that makes it controller-specific is unresolved, because the CSR is Bluetooth 4.0 and takes the legacy
LE Create Connectionpath rather than the extended one.Related prior reports, none with a fix and none identifying the discovery-state dependency: bluez#2309, bluez#2356, bleak#1244, kernel bugzilla 199111, open since 2018. Chrome also registers no BlueZ pairing agent, so pairing cannot complete on Linux without one supplied externally; there is more detail in adafruit/circuitpython#11178.
Separate from this PR: #545 fixes Save As silently corrupting files, found during the same testing.